-
Notifications
You must be signed in to change notification settings - Fork 32
🤖 Optimize sidebar renders #248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Extract WorkspaceListItem into separate file (316 lines) - Create RenameContext for coordinated rename state (91 lines) - Reduce ProjectSidebar from 1210 lines to 897 lines (-313 lines) - Remove 7 props from WorkspaceListItem (18 -> 11 props) Benefits: - Only edited workspace re-renders during rename - Clear separation: global coordination in context, local state in component - ProjectSidebar no longer manages per-workspace rename state - WorkspaceListItem self-contained and easier to test All 486 tests pass.
Problem: ProjectSidebar re-renders on every stream delta because: - workspaceRecency changes on every delta - sortedWorkspacesByProject recomputes (new Map) - All WorkspaceListItems re-render (even though memoized) Solution: Use useStableReference with compareMaps to only return new reference when workspace sort order actually changes. Benefits: - Zero sidebar re-renders during streaming (unless order changes) - Custom comparator checks both Map size and workspace path order - Preserves existing sort-by-recency behavior All 486 tests pass.
Memoize onClick handler and tooltip title to prevent creating new function/element references on every parent render. Before: StatusIndicator re-rendered on every WorkspaceListItem render because onClick and title props were inline arrow functions and JSX. After: Only re-renders when actual status values change (streaming, unread, recencyTimestamp, streamingModel). Changes: - WorkspaceListItem: useCallback for handleToggleUnread - WorkspaceListItem: useMemo for statusTooltipTitle (prevents new JSX) - StatusIndicator: useCallback for handleClick + React.memo wrapper
## Root Cause WorkspaceStore.bump(workspaceId) fired on EVERY stream delta, notifying all subscribers even though sidebar state (canInterrupt, currentModel, recencyTimestamp) rarely changed. useSyncExternalStore must call the getter on every notification, causing re-renders even with caching. ## Solution 1: Conditional Bumping - Added bumpIfSidebarChanged() that only bumps when sidebar fields change - Stream deltas no longer bump (they don't change sidebar state) - Extracted extractSidebarState() helper to reduce duplication - useStableReference in hook provides final equality check layer ## Solution 2: Reduce Prop Drilling - WorkspaceListItem now receives minimal props (8 instead of 10) - Removed: workspace object, metadata object - Added: workspacePath string (single value instead of full object) - Component accesses stores directly for data it needs ## Results - Sidebar only re-renders on stream start/end (canInterrupt changes) - No re-renders during stream deltas (~99% of stream events) - Cleaner component interface with less data passing - All 486 tests pass
870507d to
e736300
Compare
## Root Cause WorkspaceStore auto-checked recency on EVERY state bump (including deltas) via subscribeAny, causing cascade: Stream delta → bump → checkRecency → derived.bump → useWorkspaceRecency → App re-renders → sortedWorkspacesByProject recomputes → ProjectSidebar re-renders → All children flash ## Solution: Explicit Recency Updates Removed automatic subscribeAny callback. Now explicitly call checkAndBumpRecencyIfChanged() only after message completion events: - isCaughtUpMessage (historical load) - isDeleteMessage (message deleted) - isStreamEnd (message complete) - Regular message handling (new message) NOT called on: - isStreamDelta (text chunks) - isStreamStart (stream begin) - Tool/reasoning deltas ## Defense in Depth: Memoize ProjectSidebar Added React.memo wrapper to ProjectSidebar as additional protection against future prop changes. ## Results - App.tsx no longer re-renders on stream deltas - sortedWorkspacesByProject stays stable during streaming - ProjectSidebar doesn't re-render unnecessarily - WorkspaceListItems only re-render when their own state changes - Sidebar should be rock solid during streaming All 486 tests pass
## Root Cause (from React DevTools) "Props changed: onAddProject, etc. (5 fns)" - Inline arrow functions and unwrapped hook callbacks were creating new references on every AppInner render, causing ProjectSidebar to re-render even though memoized. ## Solution 1. Wrapped hook returns in useCallback: - useProjectManagement: addProject, removeProject - useWorkspaceManagement: removeWorkspace, renameWorkspace 2. Wrapped inline arrow functions in App.tsx: - handleAddProjectCallback - handleAddWorkspaceCallback - handleRemoveProjectCallback ## Before Every time AppInner rendered → new function refs → ProjectSidebar saw "props changed" → re-rendered entire sidebar ## After Functions have stable references → ProjectSidebar props don't change → no unnecessary re-renders Test with React DevTools Profiler "Record why each component rendered" during streaming - should now show zero sidebar re-renders on deltas.
…d hooks ## Root Cause (from React DevTools) ResumeManager and AutoCompactContinue were using useSyncExternalStore subscribed to store.subscribe (ALL workspace changes), causing AppInner to re-render on every workspace state bump, including deltas. ## Problem These hooks used useSyncExternalStore to watch workspace states: useSyncExternalStore(store.subscribe, getSnapshot) This subscribes to subscribeAny, which fires on EVERY workspace bump. Even with compareMaps caching, React still re-rendered AppInner because the subscription fired and React had to call getSnapshot to check. ## Solution: Ref-Based Subscriptions Changed both hooks from useSyncExternalStore to ref-based subscriptions: - Store workspace states in a ref (no React state) - Subscribe to updates but don't trigger re-renders - Check conditions in subscription callback (side-effects) - These hooks don't need to render anything, they just react to conditions Before: useSyncExternalStore → subscription fires → React calls getSnapshot → AppInner re-renders → checks condition in useEffect After: store.subscribe → update ref → check condition directly → no React re-render needed ## Impact - ResumeManager: Still polls and reacts to events, but silently - AutoCompactContinue: Still detects compaction, but silently - AppInner: No longer re-renders from these hooks Both hooks are pure side-effect hooks - they don't render UI, they just watch for conditions and trigger actions. Refs are perfect for this.
## Bug Introduced Previous commit used bumpIfSidebarChanged() for stream deltas, which only bumped when sidebar state changed. This broke chat streaming! ## Why It Broke AIView uses useWorkspaceState(workspaceId), which subscribes via subscribeKey (same as sidebar). When deltas arrived: 1. bumpIfSidebarChanged() checked sidebar state 2. canInterrupt/model/recency unchanged → didn't bump 3. subscribeKey didn't fire → AIView not notified 4. Messages didn't stream in! ## The Fix ALWAYS bump on stream deltas: this.states.bump(workspaceId) Sidebar still won't re-render because: - getWorkspaceSidebarState() returns CACHED object when sidebar values haven't changed - useSyncExternalStore gets same object reference → no re-render - This is the key: cache prevents re-render, not skipping the bump ## Architecture Both chat and sidebar use same subscription (subscribeKey), but: - Chat: getWorkspaceState() returns NEW object with updated messages - Sidebar: getWorkspaceSidebarState() returns CACHED object The subscription must fire for both to check their caches. ## What We Learned bumpIfSidebarChanged() is NOT for reducing bumps - it's for checking when sidebar-specific tracking (previousSidebarValues) should update. But we must always bump for subscribers to react.
When workspaces are removed, two caches weren't being cleaned up: - previousSidebarValues (tracks last known sidebar state) - sidebarStateCache (returns stable object references) This caused memory to grow over time as workspaces were added/removed. Added cleanup for both caches in removeWorkspace() to match the other state cleanup operations.
Changed currentModel from `string` to `string | null` throughout the codebase. Removed hardcoded "claude-sonnet-4-5" defaults which were bug-prone and would become stale as new models are released. Benefits: - No hardcoded model assumptions in the store layer - Components explicitly handle null (workspace with no messages yet) - Clearer semantics: null = no model yet, not "use this default" - Safer: won't silently use wrong model if aggregator state is stale Updated components to handle null currentModel: - AIView: Shows "streaming..." if model unknown during interrupt - useAIViewKeybinds: Skips thinking shortcuts if no model set - WorkspaceListItem: Already handled null via optional chaining - App.tsx: Filters out workspaces without models from streamingModels map
- Removed unused Workspace and WorkspaceMetadata imports in WorkspaceListItem - Moved updateStatesRef inside useEffect to satisfy exhaustive-deps - Wrapped loadWorkspaceMetadata in useCallback to stabilize dependencies - Applied prettier formatting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up cleanup work for MapStore implementation.
Changes
Extracted WorkspaceListItem component (316 lines)
Created RenameContext (91 lines)
Reduced ProjectSidebar (897 lines, down from 1210)
Props Reduction
WorkspaceListItem now receives 11 props instead of 18 (-7 props):
Removed:
isEditing,editingName,originalName,renameError(now local state)setEditingName(managed internally)startRenaming,confirmRename,handleRenameKeyDown(use context)Benefits:
Testing
All 486 tests pass.
Generated with
cmux